Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

simplify naming #925

Closed
wants to merge 38 commits into from
Closed

simplify naming #925

wants to merge 38 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented May 7, 2023 •
edited
Loading

Description

Binary names and paths were too long.

This PR:

  • interchain-security-cd => consumer
  • interchain-security-cdd => democracy
  • interchain-security-pd => provider

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • Included the correct type prefix in the PR title
  • Added ! to the type prefix if API or client breaking change
  • Targeted the correct branch (see PR Targeting)
  • Provided a link to the relevant issue or specification
  • Followed the guidelines for building SDK modules
  • Included the necessary unit and integration tests
  • Added a changelog entry to CHANGELOG.md
  • Included comments for documenting Go code
  • Updated the relevant documentation or specification
  • Reviewed "Files changed" and left comments if necessary
  • Confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage

@faddat faddat requested a review from a team as a code owner May 7, 2023 20:51
@faddat faddat marked this pull request as draft May 7, 2023 20:52
@jtremback
Copy link
Contributor

This just changes the binary names? I like the idea

@faddat
Copy link
Contributor Author

faddat commented May 9, 2023

yep. I just need to clean it up and such, should be able to get to it tomorrow.

It is inspired by typing:

interchain-security-pd

too many times

:)

@jtremback
Copy link
Contributor

I'm getting test failures on this even after re-running. Can you check that the e2e tests all have the updated binary names?

@faddat
Copy link
Contributor Author

faddat commented May 11, 2023

ah that's why it is still on draft.

@faddat faddat marked this pull request as ready for review May 11, 2023 19:28
@faddat
Copy link
Contributor Author

faddat commented May 13, 2023

hmmmm I thought i had this working....

@faddat
Copy link
Contributor Author

faddat commented May 13, 2023 •
edited
Loading

@jtremback please note that we could debug this directly on github, if we changed the automated tests to use the --verbose flag as seen in #867

that would:

  • prioritize human time over machine time
  • make agreeing on the state of bugs easier
  • reduce overall time spent to diagnose and fix issues

... idk exactly how things work at Informal but at Notional we've noticed very consistently that machines are dramatically cheaper than the people who program them.

@faddat
Copy link
Contributor Author

faddat commented May 13, 2023

For reference, 5/5 vietnamese undersea cables have some kind of fault right now.

Thus, i am still downloading docker containers.

@faddat faddat marked this pull request as draft May 13, 2023 23:27
@faddat faddat mentioned this pull request May 13, 2023
Closed
@faddat faddat marked this pull request as ready for review May 13, 2023 23:36
@jtremback
Copy link
Contributor

LGTM but looks like some merge conflicts snuck in

@shaspitz shaspitz marked this pull request as draft June 9, 2023 21:50
@shaspitz
Copy link
Contributor

shaspitz commented Jun 9, 2023

Set this to draft, but once the merge conflicts are fixed I'm happy to review

@faddat
Copy link
Contributor Author

faddat commented Jul 6, 2023

Not sure that changing "democracy consumer" to "democracy" is beneficial or makes things clearer.

yeah the idea was def to make binary names shorter, any other name changes, were done for the sake of consistency is there a specific instance where this is an issue?

Happy to address

@faddat
Copy link
Contributor Author

faddat commented Jul 6, 2023

https://github.com/cosmos/interchain-security/issues?q=is%3Aopen+is%3Aissue+label%3Arefactoring

🖖

thank you

faddat
faddat commented Jul 6, 2023
View reviewed changes
testutil/ibc_testing/specific_setup.go
@@ -14,7 +14,7 @@ import (
"github.com/cometbft/cometbft/libs/log"

appConsumer "github.com/cosmos/interchain-security/v3/app/consumer"
appConsumerDemocracy "github.com/cosmos/interchain-security/v3/app/consumer-democracy"
appConsumerDemocracy "github.com/cosmos/interchain-security/v3/app/democracy"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this what you were unsure of @smarshall-spitzbart ?

@jtremback
Copy link
Contributor

Merge conflicts snuck in again, can merge once it's fixed though

@faddat
Copy link
Contributor Author

faddat commented Aug 13, 2023

@jtremback hey sorry about the delay here. Baby gap sir.

@github-actions github-actions bot added C:Testing Assigned automatically by the PR labeler C:Build Assigned automatically by the PR labeler C:Docs Assigned automatically by the PR labeler labels Aug 20, 2023
@faddat
Copy link
Contributor Author

faddat commented Aug 20, 2023

I've merged main, hopefully this fixes the cometmocktest.

@github-actions github-actions bot added the C:x/provider Assigned automatically by the PR labeler label Sep 16, 2023
mpoke
mpoke approved these changes Sep 19, 2023
View reviewed changes
Copy link
Contributor

@mpoke mpoke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just one nit below.

docs/docs/features/key-assignment.md
@@ -63,7 +63,7 @@ OR
gaiad query provider validator-provider-key <consumer-chain-id> consumervalcons1e....123asdnoaisdao
```

You must use a `valcons` address. You can obtain it by querying your node on the consumer `consumerd tendermint show-address`
You must use a `valcons` address. You can obtain it by querying your node on the consumer `consumer tendermint show-address`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be consumerd.

@mpoke
Copy link
Contributor

mpoke commented Sep 19, 2023

@faddat Could you please also add the compiler/linker flags (ldflags) during the build call so that we can also close #1035? We decided against goreleaser for ICS, but it would be useful to populate those fields for the version CLI command.

mpoke
mpoke reviewed Sep 20, 2023
View reviewed changes
Dockerfile
COPY --from=is-builder /go/bin/providerd /usr/local/bin/providerd
COPY --from=is-builder /go/bin/consumerd /usr/local/bin/consumerd
COPY --from=is-builder /go/bin/democracyd /usr/local/bin/democracyd
COPY --from=is-builder /go/bin/sovereignd /usr/local/bin/sovereignd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also change sovereign to standalone? Calling a non-ICS chain sovereign, entails that consumer chains don't have sovereignty, which is not true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I'm good with this change sir

@github-actions github-actions bot added the C:x/consumer Assigned automatically by the PR labeler label Jan 1, 2024
@mpoke mpoke self-requested a review January 4, 2024 10:07
@mpoke
Copy link
Contributor

mpoke commented Jun 11, 2024

Closing this as it is out of sync with main. It's easier to redo the PR than to rebase.

@mpoke mpoke closed this Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shaspitz shaspitz shaspitz left review comments

@mpoke mpoke Awaiting requested review from mpoke

Assignees
No one assigned
Labels
C:Build Assigned automatically by the PR labeler C:Docs Assigned automatically by the PR labeler C:Testing Assigned automatically by the PR labeler C:x/consumer Assigned automatically by the PR labeler C:x/provider Assigned automatically by the PR labeler
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@faddat @jtremback @shaspitz @mpoke